Clickable roomId on message by kumarpalsinh25 · Pull Request #1268 · acterglobal/a3

kumarpalsinh25 · 2024-01-11T07:06:09Z

This PR fixes issue no - #1097.

Feature Reference Video:
https://github.com/acterglobal/a3/assets/51526480/009763cf-6034-43fe-8552-b33bc64656a8
Unit-test Reference Video:
https://github.com/acterglobal/a3/assets/51526480/c791f224-dcbe-4d24-856c-d51c97f68f8d

- Handle click of matrix room url pattern

app/lib/features/chat/widgets/text_message_builder.dart

gtalha07 · 2024-01-11T10:44:31Z

app/test/features/chat/chat_message_test.dart

-    });
-
-    test(
+  group(


gtalha07

Great and useful changes overall, there's one change I'd like to have prior to merging!!

…yet joined

gnunicorn

There is another format missing, and while I can ignore the questionable way to create the via-param, we need to have support for linking to spaces, too!

gnunicorn · 2024-01-11T15:18:34Z

app/lib/features/chat/chat_utils/chat_utils.dart

+    var roomIdWithServer = '$roomId:$server';
+
+    //For public groups - Replace encoded '%23' string with #
+    if (roomIdWithServer.startsWith('%23')) {
+      roomIdWithServer = roomIdWithServer.replaceAll('%23', '#');
+    }


I think you just want to use the URI class and its decoder functions here ... there could be more than just the starting character being encoded...

gnunicorn · 2024-01-11T15:22:33Z

app/lib/features/chat/pages/room_profile_page.dart

                  tiles: [
                    SettingsTile(
                      onPressed: (ctx) {
+                        final serverName = roomId.split(':').last;


This isn't really how via is meant to work. See the matrix URI spec for details. But ideally via tells you about (additional) servers that we know to be in the room as alternative ways to connect to the room (in particular if the homeserver isn't available). Just repeating the homeserver is a bit of a short-cut that doesn't in practice actually help doing the right resolution.

We can keep this for now, but should probably add a helper function on the rust side that compiles the actual correct information including via.

Ok got it. I will check for matrix specification and do required changes accordingly.

I have read the documentations, and agree with you that repeating the homeserver doesn't make sense.

I'm adding FIXME comment and once rust side helper function is ready then will change it accordingly.

gnunicorn · 2024-01-11T15:22:47Z

app/lib/features/chat/chat_utils/chat_utils.dart

+  final urlRegexp = RegExp(
+    r'https://matrix\.to/#/(?<roomId>.+):(?<server>.+)+',
+    caseSensitive: false,
+  );


matrix:// is also a valid link format: https://spec.matrix.org/v1.9/appendices/#uris

How can I get link like matrix:// from Acter or Element general?

I am not sure which clients are sending those at the moment... the element-x iOS maybe?

@gnunicorn
No, I have check element web and element iOS. Both are sending matrix.to url.

And matrix:// is not a link url,I believe It is url schema which is generally used in deep linking.

@gnunicorn

Is there any platform or way using with I can get matrix:// and test it?

If matrix:// type of url are consider as url schema then still I have to handle condition for it?

matrix.to is the old, matrix: is the new way of doing it.

@gnunicorn
Sorry but I'm not agree with that.

Not sure but based on the my knowledge matrix:// and matrix.to are different things and have their own purposes.

matrix:// is Matrix URI scheme which ideally suitable in deep-linking for direct intent calls

matrix.to is Matrix Navigation URL and widely used by all the matrix platforms but general navigations.

For better clarification, I'm looking for any platform which is using matrix:// (Matrix URI Scheme definition).
If you have any idea about such matrix platforms then can you please share it so that I can look into the same and get detail understanding.

And if you don't mind then can we proceed with this PR as this PR is mainly for copying RoomID URL from profile page and user should able to handle it with click on that message like element and other platforms doing because matrix:// is different thing and don't have any use-case at least in Acter App.

are different things and have their own purposes.

That is a misunderstanding though. Quoting the spec, about the matrix.to it says: "This namespacing existed prior to a matrix: scheme." hinting to the fact that they do the same but the existence of both is just an historically artifact to stay backwards compatible when matrix: was added: Both are used for deep-linking (clicking the matrix.to-link on a smartphone with element on it, you will see it come up), and targeting the same objects. From a purpose perspective they do and are the same. With matrix: (no double // after the :by the way) being the one for the future and matrix.to being the fallback.

Thus if want to only use one, we should do the matrix:-format and not the old one but we are only using the legacy with this PR. If you don't want to implement this now, let's open a ticket saying we need matrix:-protocol-support.

Thanks @gnunicorn for details understanding.
If matrix:// is the new way then for navigating and deep-link then I would like to integrate that way in Acter and have support for both.

@gnunicorn Do you have idea of any of the matrix platform which is using this new way? I just wanted to confirm certain use-cases with that.

Do you have idea of any of the matrix platform which is using this new way? I just wanted to confirm certain use-cases with that.

as said, if Element-X isn't using it, I wouldn't know if any does. According to the original spec proposal fluffychat and nheko have support for it (don't know if they post use it primarily though).

FYI, when I say the "new way" of doing it ... the Spec was merged 2021 .... and looks like even Element doesn't use it primarily yet, so ....

Aha, I see..
Let me create create new ticket for "matrix:-protocol-support" and then we can move on with current matrix.to support.

gnunicorn · 2024-01-11T15:23:12Z

app/lib/features/chat/widgets/text_message_builder.dart

    String parsedString = simplifyBody(widget.message.text);
    final urlRegexp = RegExp(
-      r'https://matrix\.to/#/@[A-Za-z0-9\-]+:[A-Za-z0-9\-]+\.[A-Za-z0-9\-]+',
+      r'https://matrix\.to/#/[@!#][A-Za-z0-9\-]+:[A-Za-z0-9\-]+\.[A-Za-z0-9\-]+',


matrix:// is also a valid link we need to check for https://spec.matrix.org/v1.9/appendices/#uris

gnunicorn · 2024-01-11T15:25:15Z

app/lib/features/chat/widgets/text_message_builder.dart

+    }
+  }
+
+  void askToJoinGroup(


this should probably become a system-wide route as there are more complex scenarios we need to match, where we might a) need to knock first or load data from remotely before we even know ...

Understood, let me move this function to chat_utils

gnunicorn · 2024-01-11T15:25:36Z

app/lib/features/chat/widgets/text_message_builder.dart

+        final chat = await ref.read(chatProvider(roomId).future);
+        if (!context.mounted) return;
+        if (chat.isJoined()) {
+          // Navigate to chat room
+          context.goNamed(
+            Routes.chatroom.name,
+            pathParameters: {'roomId': chat.getRoomIdStr()},
+          );
+        } else {
+          askToJoinGroup(context, ref, roomId);
+        }
+      } catch (e) {
+        askToJoinGroup(context, ref, roomId);
+      }


What if the link is for a space? Then we don't properly route there?

gnunicorn · 2024-01-11T15:26:05Z

app/test/features/chat/chat_message_test.dart

-    });
-
-    test(
+  group(


- Used URI class for better management of the urls - Separated some code for making then reusable

kumarpalsinh25 added 4 commits January 10, 2024 19:03

- Copy roomId with matrix room url pattern

b19a79d

- Handle click of matrix room url pattern

- Added unit testing for Matrix Room Id parsing

76d360f

- Updated change logs data

0bc4a5a

- Minor typo change

fd081bd

kumarpalsinh25 requested review from gnunicorn and gtalha07 January 11, 2024 07:12

gtalha07 reviewed Jan 11, 2024

View reviewed changes

app/lib/features/chat/widgets/text_message_builder.dart Outdated Show resolved Hide resolved

gtalha07 reviewed Jan 11, 2024

View reviewed changes

app/test/features/chat/chat_message_test.dart

});

test(

group(

Copy link
Copy Markdown

Contributor

gtalha07 Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!!

gtalha07 approved these changes Jan 11, 2024

View reviewed changes

- Added code to check for is room Joined and ask user to join if not …

fc3c828

…yet joined

kumarpalsinh25 requested a review from gtalha07 January 11, 2024 12:43

gnunicorn requested changes Jan 11, 2024

View reviewed changes

- Added //FIXME for ?via server-name management

603d278

- Used URI class for better management of the urls - Separated some code for making then reusable

kumarpalsinh25 requested a review from gnunicorn January 15, 2024 06:55

kumarpalsinh25 merged commit 0009900 into main Jan 15, 2024

kumarpalsinh25 deleted the kumar/issue-1097 branch January 15, 2024 13:46

-                  });
-                  test(
+                group(

Uh oh!

Conversation

kumarpalsinh25 commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtalha07 left a comment

Choose a reason for hiding this comment

Uh oh!

gnunicorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumarpalsinh25 Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumarpalsinh25 Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kumarpalsinh25 commented Jan 11, 2024 •

edited

Loading

kumarpalsinh25 Jan 12, 2024 •

edited

Loading

kumarpalsinh25 Jan 12, 2024 •

edited

Loading